-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add audit for Ruby files in taps. #17574
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There seems to be a bunch of changes here and I don't fully understand which ones are stylistic and which ones change behaviour. Can you elaborate a bit more on what this is doing, how and why?
Most of the changes are caused by moving Apart from that, the only thing that's new is the |
|
||
return if stray_ruby_files.none? | ||
|
||
problem "Ruby files in wrong location:\n#{stray_ruby_files.map(&:to_s).join("\n")}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should say what location they should be in. Even from reading the code: I don't really understand what would be the right/wrong location here. Can you elaborate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct would be *.rb
(if Formula/
doesn't exist), cmd/**/*.rb
, Formula/**/*.rb
and Casks/**/*.rb
, anything else incorrect.
Additionally, Formula/**/*.rb
is incorrect in homebrew/cask
and cask/**/*.rb
is incorrect in homebrew/core
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Homebrew/test-bot is a tap with formulae but also uses lib/**/*.rb
and spec/**/*.rb
.
Homebrew/portable-ruby has Abstract/*.rb
.
Stray files detection here seems somewhat far reaching.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additionally,
Formula/**/*.rb
is incorrect inhomebrew/cask
andcask/**/*.rb
is incorrect inhomebrew/core
.
This seems reasonable and a good add. Could also look for Ruby files that look like formulae or casks that aren't in a directory that Homebrew will ever find
Correct would be
*.rb
(ifFormula/
doesn't exist),cmd/**/*.rb
,Formula/**/*.rb
andCasks/**/*.rb
, anything else incorrect.
Agreed with @Bo98: this does not and feels like it's going to blow up on a lot of taps, official and otherwise.
What's the motivation here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The motivation is to move the logic from the cask CI workflow into brew audit
. I'm also fine with simly removing this check altogether.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which part of the cask CI currently checks this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I think either:
- this check can be removed entirely
- if we want it to be in Homebrew/brew: it should check for files than e.g. contain
class .+ < Formula
orcask ".+" do
that aren't inpotential_formula_dirs
Lines 655 to 658 in 9f58612
sig { returns(T::Array[Pathname]) } def potential_formula_dirs @potential_formula_dirs ||= [path/"Formula", path/"HomebrewFormula", path].freeze end cask_dir
Lines 668 to 671 in 9f58612
sig { returns(Pathname) } def cask_dir @cask_dir ||= path/"Casks" end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Matching class .+ < Formula
would still include the Abstract/*.rb
mentioned above. Then again, I'm not a fan of subclassing Formula
that way in the first place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then: remove it entirely, I think.
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
brew style
with your changes locally?brew typecheck
with your changes locally?brew tests
with your changes locally?Replaces the same logic in the
homebrew/cask
CI workflow with an audit inTapAuditor
.See Homebrew/homebrew-cask#177890 (comment).